-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(i18n): import psuedo-locale json from TC #5726
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool cool cool
should we add these locales to the cli-flags + typings + index.js? if not, do we want to import them at all or was this just testing :)
also they seem to use _
while I was using -
is there a best practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also want to update https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/lib/locales/index.js
and should
lighthouse/lighthouse-cli/cli-flags.js
Lines 122 to 125 in e99673b
.choices('locale', [ | |
'en-US', // English | |
'en-XA', // Accented English, good for testing | |
]) |
be based on
Object.keys(require('...../locales/index.js'))
?
found another TC export script that has an option for hyphen rather than underscore. So this is a common thing. :) Since our build script also prefers hyphen i guess we should go with that.. |
48d4206
to
de7623d
Compare
:/ ya the extra locales will be useless if we can't actually generate reports with them |
🏏 🏏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -58,7 +58,8 @@ describe('i18n', () => { | |||
describe('#getRendererFormattedStrings', () => { | |||
it('returns icu messages in the specified locale', () => { | |||
const strings = i18n.getRendererFormattedStrings('en-XA'); | |||
expect(strings.passedAuditsGroupTitle).toEqual('P̂áŝśêd́ âúd̂ít̂ś'); | |||
expect(strings.passedAuditsGroupTitle).toEqual('[Þåššéð åûðîţš one two]'); | |||
expect(strings.scorescaleLabel).toEqual('[Šçöŕé šçåļé: one two]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably convert this file back to assert
at some point rather than having a different assertion library just for this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nooooooooooooo 😢 assert is so sad compared to expect
updated